use services.Ticker #12668
use services.Ticker #12668
Conversation
|
I see you updated files related to
|
|
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
jmank88
force-pushed
the
services-ticker
branch
2 times, most recently
from
bc38e5e to
c802511
Compare
| @@ -120,6 +120,7 @@ func (o *zkSyncL1Oracle) run() { | |||
| func (o *zkSyncL1Oracle) refresh() (t *time.Timer) { | |||
| t, err := o.refreshWithError() | |||
| if err != nil { | |||
| o.logger.Criticalw("Failed to refresh gas price", "err", err) | |||
There was a problem hiding this comment.
@simsonraj I had trouble debugging this PR, because these refresh methods are pushing errors to mark the service unhealthy, but without logging anything. These are typically paired with a critical log. Is that appropriate to do in this case, and the other files in this PR?
There was a problem hiding this comment.
Good catch, I think its appropriate to append an error, but not sure if it needs to be critical if it is polling. At least for zkSync case, it may not be since we dont need gasPrice exactly real-time & not sure how much impact an RPC malfunction will have here. Looking at Arbitrum it seems the intent was to log it as an error.
May be @amit-momin @FelixFan1992, any opinions?
There was a problem hiding this comment.
What if we looked at the timestamp, and only took this action (both log and unhealthy) if we cannot refresh after some threshold of time has passed?
There was a problem hiding this comment.
Logging the error definitely makes sense here. Following the example of other gas estimator code, I think it's ok to log Critical here. If there's an RPC issue, it's likely a connectivity issue which we'd want to surface to NOPs urgently. Although I'm sure other components would also start making some noise for it.
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Requires: